Skip to content

test: SharedState collective save-failure rollback (PRPUNDIT-8, PRPUNDIT-9) - #1324

Open
jiagaoxiang wants to merge 4 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-9
Open

test: SharedState collective save-failure rollback (PRPUNDIT-8, PRPUNDIT-9)#1324
jiagaoxiang wants to merge 4 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-9

Conversation

@jiagaoxiang

@jiagaoxiang jiagaoxiang commented Aug 28, 2026

Copy link
Copy Markdown

Summary

  • Forces SharedState.save to fail during record_collective and record_collective_integration.
  • Asserts in-memory last_collective / collective_attempts restore and on-disk state.json is unchanged.

Covers test gaps PRPUNDIT-8 and PRPUNDIT-9 (merged from #1323 into this PR).

Test plan

  • PYTHONPATH=src pytest src/hyperloom/inference_optimizer/tests/test_collective_lane.py::test_record_collective_rolls_back_when_save_fails src/hyperloom/inference_optimizer/tests/test_collective_lane.py::test_record_collective_integration_rolls_back_when_save_fails

…IT-8)

Force SharedState.save to fail and assert in-memory last_collective/collective_attempts and state.json are unchanged.
Force SharedState.save to fail and assert in-memory last_collective/collective_attempts and state.json are unchanged.
@jiagaoxiang
jiagaoxiang requested a review from a team as a code owner August 28, 2026 07:13
Keep both SharedState save-failure rollback tests in one file so the pair can land without a conflict.
@jiagaoxiang

Copy link
Copy Markdown
Author

Merged #1323 into this branch and resolved the test_collective_lane.py conflict. This PR now has both record_collective and record_collective_integration save-failure tests. Safe to merge after or instead of #1323.

@jiagaoxiang jiagaoxiang changed the title test: roll back record_collective when save fails (PRPUNDIT-9) test: SharedState collective save-failure rollback (PRPUNDIT-8, PRPUNDIT-9) Aug 28, 2026

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with an independent verification pass: ran the touched test file on this branch and mutation-tested the production code to confirm the new tests actually catch regressions (not just pass).

Mutations tried and caught: deleting the restore lines in record_collective and in record_collective_integration each turns the corresponding new test red, so both producers are pinned separately.

No regressions in the surrounding suite (pre-existing failures verified identical on origin/main). LGTM.

@zoroyihan7
zoroyihan7 enabled auto-merge (squash) August 31, 2026 07:41
@zoroyihan7

Copy link
Copy Markdown
Contributor

/retest

(e2e failed on the Dispatch + monitor single-GPU smoke run step, which is failing on unrelated PRs too — retriggering. Auto-merge is armed, so this will land on its own once e2e is green.)

@zoroyihan7 zoroyihan7 added the skip-e2e-test It's a PR that doesn't need to be e2e tested label Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-e2e-test It's a PR that doesn't need to be e2e tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants